new(metrics): add `rules_counters_enabled` option #3192

incertum · 2024-05-13T20:51:15Z

Intended to replace https://github.com/falcosecurity/falco-exporter when used with Prometheus output

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

new(metrics): add rules_counters_enabled option

Intended to replace https://github.com/falcosecurity/falco-exporter when used with Prometheus output.
CC @leogr and @jasondellaluce (current maintainers of https://github.com/falcosecurity/falco-exporter).

Which issue(s) this PR fixes:

Fixes #3132

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

new(metrics): add `rules_counters_enabled` option

github-actions · 2024-05-13T20:51:43Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

incertum · 2024-05-13T20:52:04Z

Proposing

/milestone 0.38.0

so that we can start the deprecation cycle for https://github.com/falcosecurity/falco-exporter @leogr .

incertum · 2024-05-13T20:56:34Z

@sgaist this PR includes super minor touch ups re variable naming for the Prometheus webserver code.
In general once we start the dev cycle for Falco 0.39.0 we should chat re how to consolidate and simplify the Falco metrics model even more.

leogr · 2024-05-14T08:27:16Z

Proposing

/milestone 0.38.0

so that we can start the deprecation cycle for https://github.com/falcosecurity/falco-exporter @leogr .

If we are able to get the PR reviewed in time, +1 for me. I'll take a look as soon as I can.

/assign
/assign @FedeDP
(as discussed)

FedeDP · 2024-05-14T11:26:10Z

userspace/falco/falco_metrics.cpp

+				auto rule = rules.at(i);
+				std::string rules_metric_name = "rules." + rule->name;
+				// Separate processing of rules counter metrics given we add extra tags
+				auto metric = libs_metrics_collector.new_metric(rules_metric_name.c_str(),


Out of scope for this PR; but since we only instantiate libs_metrics_collector to add metrics to it, that, given the code: https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/metrics_collector.h#L260
seems like it can be made static, if we made the change in libs (ie: make new_metric static), can we drop the libs_metrics_collector instance?

Yup, I'll create an issue later to track all the post Falco 0.38.0 metrics items.

FedeDP · 2024-05-14T11:27:11Z

userspace/falco/falco_metrics.cpp

@@ -86,26 +86,59 @@ std::string falco_metrics::to_text(const falco::app::state& state)
 		{
 			prometheus_text += prometheus_metrics_converter.convert_metric_to_text_prometheus("evt_source", "falcosecurity", "falco", {{"evt_source", source}});
 		}
-		std::vector<metrics_v2> static_metrics;
-		static_metrics.push_back(libs_metrics_collector.new_metric("start_ts",
+		std::vector<metrics_v2> falco_metrics;


It would be nice to split this method into multiple logical pieces, like:

scap_engines_to_text()

falco_rules_to_text()

agent_info_to_text()

...and so on.
Do you think it is feasible?

Of course. Big question is now or wait until a more major cleanup follows post Falco 0.38.0, cc @sgaist?

Don't have strong opinion; for me, the sooner the better (helps code readability), but we can postpone it without any problem.

Hey @FedeDP was looking into it and it would have made things more ugly. We need a proper new falco metrics model as @sgaist suggested somewhere here #3140 and think more carefully how to split things.

Meanwhile, I added some more comments, is this acceptable @FedeDP 🙃 ?

Much better, thanks!

userspace/engine/stats_manager.h

sgaist · 2024-05-14T11:42:42Z

userspace/engine/stats_manager.h

 	std::vector<std::unique_ptr<std::atomic<uint64_t>>> m_by_priority;
 	std::vector<std::unique_ptr<std::atomic<uint64_t>>> m_by_rule_id;


Outside of this PR scope but why unique pointers to atomic rather than just atomic ?

cc @jasondellaluce ?

FedeDP

/approve

poiana · 2024-05-16T12:58:45Z

LGTM label has been added.

Git tree hash: 66ff78a9dd26a9b03a03f354cd1bedb00612fff1

sgaist · 2024-05-16T15:27:26Z

userspace/falco/falco_metrics.cpp

+			for (size_t i = 0; i < rule_stats_manager.get_by_rule_id().size(); i++)
+			{
+				auto rule = rules.at(i);
+				std::string rules_metric_name = "rules." + rule->name;
+				// Separate processing of rules counter metrics given we add extra tags
+				auto metric = libs_metrics_collector.new_metric(rules_metric_name.c_str(),
+																	METRICS_V2_RULE_COUNTERS,
+																	METRIC_VALUE_TYPE_U64,
+																	METRIC_VALUE_UNIT_COUNT,
+																	METRIC_VALUE_METRIC_TYPE_MONOTONIC,
+																	rule_stats_manager.get_by_rule_id()[i]->load());


What about minimizing the number of calls to get_by_rule_id ?

Suggested change

for (size_t i = 0; i < rule_stats_manager.get_by_rule_id().size(); i++)

{

auto rule = rules.at(i);

std::string rules_metric_name = "rules." + rule->name;

// Separate processing of rules counter metrics given we add extra tags

auto metric = libs_metrics_collector.new_metric(rules_metric_name.c_str(),

METRICS_V2_RULE_COUNTERS,

METRIC_VALUE_TYPE_U64,

METRIC_VALUE_UNIT_COUNT,

METRIC_VALUE_METRIC_TYPE_MONOTONIC,

rule_stats_manager.get_by_rule_id()[i]->load());

auto rules_by_id = rule_stats_manager.get_by_rule_id();

for (size_t i = 0; i < rules_by_id.size(); i++)

{

auto rule = rules.at(i);

std::string rules_metric_name = "rules." + rule->name;

// Separate processing of rules counter metrics given we add extra tags

auto metric = libs_metrics_collector.new_metric(rules_metric_name.c_str(),

METRICS_V2_RULE_COUNTERS,

METRIC_VALUE_TYPE_U64,

METRIC_VALUE_UNIT_COUNT,

METRIC_VALUE_METRIC_TYPE_MONOTONIC,

rules_by_id[i]->load());

Incorporated, thanks.

poiana · 2024-05-16T15:35:32Z

@sgaist: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Intended to replace https://github.com/falcosecurity/falco-exporter when used with Prometheus output Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Co-authored-by: Samuel Gaist <samuel.gaist@idiap.ch> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum · 2024-05-16T22:38:51Z

Note that just occurred to me: Let me check in the morning if white spaces in the rules names cause issues for Prometheus. Perhaps let's replace white spaces of the rules names with underscore? Other ideas?

sgaist · 2024-05-17T07:25:21Z

Note that just occurred to me: Let me check in the morning if white spaces in the rules names cause issues for Prometheus. Perhaps let's replace white spaces of the rules names with underscore? Other ideas?

Based on the OpenMetrics text format the metric name should follow these rules:

metricname = metricname-initial-char 0*metricname-char

metricname-char = metricname-initial-char / DIGIT
metricname-initial-char = ALPHA / "_" / ":"

So white spaces would indeed be wrong. As a safety measure, I would recommend turning anything that does not fit the metric name definition into an underscore.

sgaist · 2024-05-17T07:30:47Z

userspace/engine/falco_utils.cpp

+std::string sanitize_metric_name(const std::string& name)
+{
+	std::string sanitized_name = name;
+	RE2::GlobalReplace(&sanitized_name, "\\s+", "_");


Are we sure that only white spaces are the problematic characters that can be found in a metric name ?

i read your comment after I pushed lol, repushed just now.

incertum · 2024-05-17T07:34:52Z

@sgaist and @FedeDP ok added the proper helper (also for the config and rules sha256 metric names, just in case). I was about to go to sleep yesterday and then that thought came into my head. Good we fix that before merging indeed ...

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum · 2024-05-17T07:39:47Z

A rule name like Testing rule 2 (CVE-2244) becomes falco.rules.Testing_rule_2_CVE_2244 now.

or for Prometheus:

falcosecurity_falco_rules_Testing_rule_2_CVE_2244_total

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

FedeDP

/approve

poiana · 2024-05-17T08:06:50Z

LGTM label has been added.

Git tree hash: 62368cf1074053cda7d7b3356a98a1f9bdb62dd7

poiana · 2024-05-17T08:36:57Z

@sgaist: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

poiana · 2024-05-17T12:38:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP, incertum, LucaGuerra, sgaist

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP,LucaGuerra,incertum]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

incertum · 2024-05-17T12:54:44Z

/unhold

poiana added release-note kind/feature area/engine dco-signoff: yes approved labels May 13, 2024

poiana requested review from Kaizhe and leogr May 13, 2024 20:51

poiana added the size/L label May 13, 2024

poiana added the do-not-merge/hold label May 13, 2024

poiana added this to the 0.38.0 milestone May 13, 2024

poiana assigned FedeDP and leogr May 14, 2024

FedeDP reviewed May 14, 2024

View reviewed changes

sgaist reviewed May 14, 2024

View reviewed changes

incertum force-pushed the rule-counters-metrics branch from 1e60496 to a4fd515 Compare May 14, 2024 19:20

incertum mentioned this pull request May 14, 2024

[TRACKING] metrics framework future refactors / cleanups #3194

Open

FedeDP previously approved these changes May 16, 2024

View reviewed changes

poiana added the lgtm label May 16, 2024

sgaist requested changes May 16, 2024

View reviewed changes

incertum dismissed FedeDP’s stale review via f9fd0a3 May 16, 2024 19:30

poiana removed the lgtm label May 16, 2024

poiana requested a review from FedeDP May 16, 2024 19:30

new(metrics): add rules_counters_enabled option

855110c

Intended to replace https://github.com/falcosecurity/falco-exporter when used with Prometheus output Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum added 2 commits May 16, 2024 21:15

cleanup: add getter functions to stats_manager

879de06

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(metrics): improve comments

3d3e566

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the rule-counters-metrics branch from f9fd0a3 to 02c112a Compare May 16, 2024 21:15

cleanup(metrics): simplify some logic

ea01751

Co-authored-by: Samuel Gaist <samuel.gaist@idiap.ch> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the rule-counters-metrics branch from 02c112a to ea01751 Compare May 16, 2024 21:36

sgaist reviewed May 17, 2024

View reviewed changes

incertum force-pushed the rule-counters-metrics branch from 0b03b07 to 805b552 Compare May 17, 2024 07:34

incertum force-pushed the rule-counters-metrics branch from 805b552 to ea01751 Compare May 17, 2024 07:38

clenaup: add sanitize_metric_name helper

f93992f

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(test): add test cases for sanitize_metric_name

0ef8bf5

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

FedeDP approved these changes May 17, 2024

View reviewed changes

poiana added the lgtm label May 17, 2024

sgaist approved these changes May 17, 2024

View reviewed changes

LucaGuerra approved these changes May 17, 2024

View reviewed changes

poiana assigned LucaGuerra May 17, 2024

poiana removed the do-not-merge/hold label May 17, 2024

poiana merged commit b0f352e into falcosecurity:master May 17, 2024
35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new(metrics): add `rules_counters_enabled` option #3192

new(metrics): add `rules_counters_enabled` option #3192

incertum commented May 13, 2024 •

edited

Loading

github-actions bot commented May 13, 2024

incertum commented May 13, 2024

incertum commented May 13, 2024

leogr commented May 14, 2024

FedeDP May 14, 2024

incertum May 14, 2024

FedeDP May 14, 2024

incertum May 14, 2024 •

edited

Loading

FedeDP May 14, 2024

incertum May 14, 2024

FedeDP May 15, 2024

sgaist May 14, 2024 •

edited

Loading

leogr May 14, 2024

FedeDP left a comment

poiana commented May 16, 2024

sgaist May 16, 2024

incertum May 16, 2024

poiana commented May 16, 2024

incertum commented May 16, 2024

sgaist commented May 17, 2024

sgaist May 17, 2024

incertum May 17, 2024

incertum commented May 17, 2024

incertum commented May 17, 2024 •

edited

Loading

FedeDP left a comment

poiana commented May 17, 2024

poiana commented May 17, 2024

poiana commented May 17, 2024

incertum commented May 17, 2024

		std::vector<std::unique_ptr<std::atomic<uint64_t>>> m_by_priority;
		std::vector<std::unique_ptr<std::atomic<uint64_t>>> m_by_rule_id;

new(metrics): add rules_counters_enabled option #3192

new(metrics): add rules_counters_enabled option #3192

Conversation

incertum commented May 13, 2024 • edited Loading

github-actions bot commented May 13, 2024

incertum commented May 13, 2024

incertum commented May 13, 2024

leogr commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgaist May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poiana commented May 16, 2024

incertum commented May 16, 2024

sgaist commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented May 17, 2024

incertum commented May 17, 2024 • edited Loading

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 17, 2024

poiana commented May 17, 2024

poiana commented May 17, 2024

incertum commented May 17, 2024

new(metrics): add `rules_counters_enabled` option #3192

new(metrics): add `rules_counters_enabled` option #3192

incertum commented May 13, 2024 •

edited

Loading

incertum May 14, 2024 •

edited

Loading

sgaist May 14, 2024 •

edited

Loading

incertum commented May 17, 2024 •

edited

Loading